Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(closes #555) Fall back to default logic in useNativeTypes mode for RDF numbers which are not JSON numbers #625

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

(closes #555) Fall back to default logic in useNativeTypes mode for RDF numbers which are not JSON numbers #625

wants to merge 10 commits into from

Conversation

anatoly-scherbakov
Copy link
Contributor

@anatoly-scherbakov anatoly-scherbakov commented Jan 12, 2025 •
edited by pr-preview bot
Loading

This is my second attempt to fix this issue after #619.

Preview | Diff

@anatoly-scherbakov anatoly-scherbakov linked an issue Jan 12, 2025 that may be closed by this pull request
blind spot in RDF to object conversion #555
Open
@anatoly-scherbakov anatoly-scherbakov mentioned this pull request Jan 12, 2025
Closed
gkellogg
gkellogg requested changes Jan 12, 2025
View reviewed changes
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes, but this direction seems like the way to go.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@anatoly-scherbakov anatoly-scherbakov force-pushed the 555-blind-spot-in-rdf-to-object-conversion branch from fd7684b to 3b61a1b Compare January 13, 2025 19:52
TallTed
TallTed reviewed Jan 14, 2025
View reviewed changes
index.html Outdated Show resolved Hide resolved
@BigBlueHat BigBlueHat added the class-3 Class-3 change label Jan 15, 2025
@anatoly-scherbakov
Copy link
Contributor Author

anatoly-scherbakov commented Jan 15, 2025 •
edited
Loading

  • Todo: implement <ins> & <del> tags

@w3cbot
Copy link

w3cbot commented Jan 15, 2025

This was discussed during the #json-ld meeting on 15 January 2025.

View the transcript

w3c/json-ld-api#625

anatoly-scherbakov: this is the new version of w3c/json-ld-api#619 ; thanks gkellogg and pchampin for your suggestions.

gkellogg: it's a fairly small change.
… However, it is not purely editorial, so it should be turned into a candidate amendment, with <ins>s, <del>s...
… It has to be done manually, not much automation is available for that.

anatoly-scherbakov: I can not wrap a whole block in <ins> or <del>, right?

bigbluehat: correct, they are 'inline'. Is there a specific class to use?

gkellogg: there are other things to put in place. Respec documentation for them is not great, but there are examples in the same doc.

anatoly-scherbakov: what is the use-case for this marking?

gkellogg: it is there for reviewers of the specification, because we are editing a published Recommendation.

anatoly-scherbakov: an HTML diff will not be enough for the reviewers?

gkellogg: no, they do not expect to look at HTML raw code.
… You can look at the W3C process documents that describes the requirements for these things.

<gkellogg> https://www.w3.org/policies/process/#candidate-amendments

bigbluehat: this is required until we recharter. The alternative is to keep a bunch of open PR and merge them only after we recharter.
… I don't know why TallTed's tick is not green.
… In the DID WG, we are using a list of code owners.

@gkellogg
Copy link
Member

There’s additional markup needed to show the controls for candidate change ins/del.

gkellogg
gkellogg approved these changes Jan 25, 2025
View reviewed changes
Copy link
Member

@gkellogg gkellogg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the other markup necessary, and called it Candidate Correction 5.

@gkellogg gkellogg mentioned this pull request Jan 26, 2025
Merged
@anatoly-scherbakov
Copy link
Contributor Author

Thanks for reviews!

I still can't merge the PR though 🤔

image

@gkellogg gkellogg force-pushed the 555-blind-spot-in-rdf-to-object-conversion branch from 62e7730 to 33a652b Compare January 27, 2025 22:44
@gkellogg
Copy link
Member

Needs a review from @pchampin.

pchampin
pchampin approved these changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@pchampin pchampin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I'm approving this PR because the spec text now looks good, and to avoid more delay in back-and-forth...

However, the new test must be amended per my comment, as currently it is both buggy and non-compliant.

tests/fromRdf/0027-out.jsonld Outdated Show resolved Hide resolved
tests/fromRdf/0027-in.nq Outdated Show resolved Hide resolved
TallTed
TallTed suggested changes Jan 29, 2025
View reviewed changes
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny language fixes

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
anatoly-scherbakov and others added 2 commits February 9, 2025 20:23
@anatoly-scherbakov @TallTed
#555 Commit suggestions © @TallTed
065444a 
Co-authored-by: Ted Thibodeau Jr <[email protected]>
@anatoly-scherbakov @pchampin
#555 Fixes to test 0027 © @pchampin
10c39a9 
Co-authored-by: Pierre-Antoine Champin <[email protected]>
@anatoly-scherbakov
Copy link
Contributor Author

@TallTed @pchampin thanks for the reviews!

@pchampin I've applied the suggestions; indeed, the spec makes no mention of xsd:decimal, I do not exactly recall why I even added it to the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gkellogg gkellogg gkellogg approved these changes

@pchampin pchampin pchampin approved these changes

@TallTed TallTed TallTed approved these changes

Assignees
No one assigned
Labels
class-3 Class-3 change
Projects
JSON-LD Management
Status: PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

blind spot in RDF to object conversion
6 participants
@anatoly-scherbakov @w3cbot @gkellogg @pchampin @TallTed @BigBlueHat